perf(source-gen): bound generated test-entry methods (data-driven startup JIT -45%) - #6909

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

Merge Risk: ⚪ Minimal · up to 2fcf2

No identified issue needs resolution before merging; the changed generation paths retain the intended entry ordering and chunking behavior.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 66948

The generated entries are now built in smaller methods, but they retain the existing registration and factory behavior. No new security exposure was identified. Security-review coverage is incomplete, so the assessment is low risk rather than minimal risk.

Retained concerns
No architecture-level concerns identified.

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit checks each entry row
Ten nested hops before we go
A hundred fit within each run
Fill methods set them one by one
The snapshots show the paths in view
And tests count every burrow through}

_{Comment @coderabbitai help to get the list of available commands.}

Confidence Score: 5/5

[Medium risk] Refactors test entry generation to split large initializers into chunks.

The PR appears safe to merge; no new actionable issue or outstanding finding remains.

Summary

The PR splits generated test-entry initialization into smaller methods for data-driven classes. Since the previous review, it also caps each fill method at 100 total entries and adds a fixture covering a long run of plain tests.

• The new boundary preserves entry order and fills the generated array.
• No new actionable issue was identified.

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Class methods] --> B{More than 10 nested entries?}
  B -- No --> C[Single array initializer]
  B -- Yes --> D[Allocate Entries array]
  D --> E[Fill methods: at most 10 nested and 100 total entries each]

_{Reviews (3) · Last reviewed commit: "Cap total entries per fill method at 100"}

Code Review — PR #6909

This PR splits the per-class generated `Entries` array into chunked `__FillEntriesN` methods (batches of ~10 entries) to reduce JIT compilation time for data-driven test classes with many entries. Changes are confined to `src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs` plus 11 mechanically regenerated `.verified.txt` snapshots.

Review approach: Traced the chunk-boundary math, `CodeWriter` indentation/brace state across the refactor, the `HasNestedEntryConstruction` heuristic against every `PreGenerate*` helper it depends on (confirming `categories`/`properties`/`dependsOn` array literals are correctly excluded since they contain no nested calls), naming-collision risk for the new `__CreateEntries`/`__FillEntriesN` methods, evaluation-order/side-effect preservation, and AOT/trim-safety parity with the existing `__CreateInstance`/`__Invoke`/`__Attributes` generated methods. Cross-checked the regenerated `ArgsAsArrayTests.Test.verified.txt` and `MatrixTests` snapshots (13-entry classes splitting correctly into indices 0–9 and 10–12) to confirm the generator's actual output matches the intended behavior.

Findings: None. No correctness bugs, dropped invariants, or violations of the project's AGENTS.md constraints (source-generator/engine parity doesn't apply here since this is generator-only output shaping; snapshot `.verified.txt` files were updated as required, no `.received.txt` committed).

Minor observation (non-blocking): There's no test targeting the exact chunk boundary (10 vs. 11 entries, with/without nested construction) in isolation — coverage is incidental via existing large test-project classes that happen to cross the boundary. Not a defect in the diff as written, just a potential gap if the chunk size constant is ever tuned in the future.

🤖 Generated with Claude Code

Code Review — PR #6909

Reviewed the source-generator chunking change in src/TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs (ShouldChunkEntries, WriteChunkedEntries, WriteTestEntryFactoryCall, HasNestedConstruction), the new EntryChunkingTests fixtures, and all updated .verified.txt snapshots.

What I checked

• Hand-traced the chunkStarts boundary math against the checked-in snapshots (InterleavedEntryTests, ElevenDataDrivenEntryTests, ArgsAsArrayTests/MatrixTests/DecimalArgumentTests) — matches exactly, including the interleaved-plain-runs edge case.
• Confirmed CodeWriter indentation semantics support the new entries[i] = ... + WriteTestEntryFactoryCall pattern correctly.
• TestEntryFactory.CreateWithClassMetadata has no ordering side effects, so splitting the single array initializer into __FillEntriesN methods preserves identical evaluation order/indices.
• Static field initialization order (__classMetadata, __createInstance, __invoke, __attributes before Entries) is unchanged, so Entries = __CreateEntries(); still runs after its dependencies.
• This is source-generator-only; nothing about collected metadata changes (only how generated code populates the array), so no TUnit.Engine reflection-mode parity work is needed per AGENTS.md, and none was touched.
• EquatableArray<T>.AsArray() is a cheap no-alloc accessor, not LINQ.
• Snapshot updates are committed as .verified.txt only; no stray .received.txt.
• Test coverage is good: boundary cases at 10 vs 11 entries, plain-only, mixed, and interleaved constructions.

Result: No correctness bugs, regressions, or convention violations found. This aligns with the existing Codex and CodeRabbit automated reviews on this PR, which also reported no actionable findings — nothing outstanding from those to reconcile.

Approving from a correctness/design standpoint; nice minimal fix for the IL-stack-spill issue with solid boundary-condition test coverage.

Review: bound generated test-entry methods (#6909)

Solid, well-measured performance fix. Summary of what I checked:

Correctness of the chunking logic (TestMetadataGenerator.cs, WriteChunkedEntries/ShouldChunkEntries/HasNestedConstruction): traced the boundary math by hand for 0/1/10/11 nested entries and for mixed plain/nested runs. Chunk starts are always monotonic, the first chunk always begins at 0, and the final chunk always ends at methods.Length, so there are no gaps, overlaps, or empty __FillEntriesN methods. WriteTestEntryFactoryCall is a faithful extraction of the previous inline loop body — every constructor argument (data sources, dependencies, method metadata, classMetadata/createInstance/invokeBody/methodIndex/createAttributes/attributeGroupIndex) is preserved identically between the chunked and non-chunked paths, just parameterized by the terminator (, vs ;).

Iteration history: the PR's three commits show the design tightening itself in exactly the right direction — from "chunk whenever nested-construction entries exceed 10" to also capping total entries per fill method at 100, which correctly closes the gap where a chunked class with a long run of cheap plain tests could otherwise dump hundreds of factory calls into a single __FillEntriesN. That was the one real edge case in the design, and it's already fixed with a dedicated fixture (ManyPlainOneDataDrivenEntryTests-style test with ~100 plain entries) and a clear commit message ("Cap total entries per fill method at 100") explaining the reasoning with numbers.

Engine/generator parity: TestEntryFactory.CreateWithClassMetadata and the Entries field's lazy-factory registration in SourceRegistrar were unchanged and untouched by reflection-mode code — this only changes the shape of generated source, not runtime metadata, so no TUnit.Engine counterpart is needed here, consistent with AGENTS.md.

AOT/trimming/hot-path constraints: no reflection introduced; all new logic (ShouldChunkEntries, WriteChunkedEntries) runs at compile time inside the Roslyn generator, not in any discovery/execution hot path.

Tests: good coverage — snapshot tests for the 10/11 nested-entry boundary, plain-only, interleaved plain/nested, and the new 100-entry total cap, plus TUnit.TestProject fixtures marked [EngineTest(ExpectedResult.Pass)] that exercise the generated code at runtime, not just via generator snapshots.

I didn't find any functional bugs, ordering hazards, or convention violations. No blocking changes requested.

Minor, non-blocking observations:

• NestedEntriesPerFillMethod (10) and EntriesPerFillMethod (100) are similarly named constants with very different roles (one bounds expensive entries, the other bounds all entries in a chunk). The doc comments already explain this well, but a future reader skimming just the names could conflate them — not worth blocking on given the existing comments.
• The tests/TUnit.TestProject/EntryChunkingTests.cs fixtures are necessarily repetitive (many near-identical [Test] methods to hit exact entry counts); that's inherent to this kind of boundary testing and reads fine as-is.

Nice work isolating the JIT-cost root cause (stack-spilled temporaries from nested constructor calls) and validating the chunk size empirically rather than guessing.